Skip to content

registry: add AWS registry implementation#324

Merged
sandeepvinayak merged 11 commits intosalesforce:mainfrom
iamabhilaksh:registry/implement-aws-registry
Mar 9, 2026
Merged

registry: add AWS registry implementation#324
sandeepvinayak merged 11 commits intosalesforce:mainfrom
iamabhilaksh:registry/implement-aws-registry

Conversation

@iamabhilaksh
Copy link
Contributor

@iamabhilaksh iamabhilaksh commented Mar 4, 2026

AWS Registry Implementation + OCI Client Interceptor Refactoring + Region Support

Summary

The primary change in this PR is the full implementation of AwsRegistry for AWS Elastic
Container Registry (ECR). As a result of this implementation, two related refactoring changes
were required: moving AuthStrippingInterceptor to the correct module, and adding region
support to ContainerRegistryClient.


Changes

1. AwsRegistry implementation (primary change)

New full implementation of AbstractRegistry for AWS ECR.

  • Authenticates via ECR's GetAuthorizationToken API returning a Base64-encoded token
  • Token is cached in memory and proactively refreshed at the halfway point of its 12-hour
    validity window (6 hours)
  • Falls back to the cached token if a refresh fails, rather than failing the request
  • Lazily initializes EcrClient with double-checked locking for thread safety
  • Supports credentialsOverrider for custom AWS credential injection
  • Builder validates registryEndpoint and region at build time
  • pom.xml updated with required AWS SDK ECR dependency

2. AuthStrippingInterceptor moved to registry-aws (required by AwsRegistry)

AwsRegistry needs an interceptor that strips the Authorization header when redirected
to S3 for blob downloads (ECR serves layer blobs via pre-signed S3 URLs). This logic was
already present as a nested static class inside OciRegistryClient in the shared
registry-client module, which was incorrect — it is purely AWS-specific behaviour.

  • Extracted AuthStrippingInterceptor into a top-level class in registry-aws
  • Removed the nested class from OciRegistryClient
  • OciRegistryClient now fetches interceptors dynamically via AbstractRegistry.getInterceptors()
  • AwsRegistry overrides getInterceptors() to return AuthStrippingInterceptor
  • GcpRegistry uses the default empty list

3. Region support in ContainerRegistryClient (required by AwsRegistry)

AwsRegistry requires a region to construct the ECR client, but ContainerRegistryClientBuilder
had no way to pass one — causing AwsRegistry to fail at build time with AWS region is required.

Pattern followed: identical to IamClient / AbstractIam in this codebase.

  • AbstractRegistry.Builder — added withRegion(String) and a protected String region field
  • AbstractRegistry — added a protected final String region instance field assigned from the
    builder in the base constructor, mirroring AbstractIam
  • AwsRegistry — reads inherited region from AbstractRegistry; no private copy needed
  • ContainerRegistryClientBuilder — added withRegion(String) delegating to the underlying
    registry builder

Tests

  • AwsRegistryTest — full unit test coverage: auth token caching, proactive refresh, fallback
    on refresh failure, getInterceptors(), close(), builder validation for missing required
    fields, credentialsOverrider, region handling
  • AuthStrippingInterceptorTest — dedicated test class covering parameterized host matching,
    header stripping, null host handling, and case-insensitive host comparisons

Design Notes

  • Why region in the base class? Region is AWS-specific but exposed at the
    ContainerRegistryClientBuilder level so callers remain provider-agnostic. GcpRegistry
    silently ignores it — same contract as IamClient.withRegion() already in this SDK.
  • Why not extract region from the endpoint URL? Fragile — couples the implementation to
    AWS URL format conventions, breaks for PrivateLink/FIPS endpoints, and does not generalise
    to other providers.
  • Why move AuthStrippingInterceptor? Separation of concerns — the shared registry-client
    module should have no knowledge of AWS-specific HTTP behaviour.

Add AwsRegistry, an AWS-specific implementation of AbstractRegistry that
uses ECR token-based authentication. Add AuthStrippingInterceptor to strip
Authorization headers for non-registry hosts, preventing credential leakage
on redirects. Add unit tests for both classes, and add junit-jupiter-params
test dependency to registry-aws pom.xml.
@codecov-commenter
Copy link

codecov-commenter commented Mar 4, 2026

Codecov Report

❌ Patch coverage is 78.09524% with 23 lines in your changes missing coverage. Please review.
✅ Project coverage is 81.99%. Comparing base (be14812) to head (a266b07).

Files with missing lines Patch % Lines
...lesforce/multicloudj/registry/aws/AwsRegistry.java 80.24% 13 Missing and 3 partials ⚠️
.../multicloudj/registry/driver/AbstractRegistry.java 25.00% 3 Missing ⚠️
...loudj/registry/client/ContainerRegistryClient.java 0.00% 2 Missing ⚠️
...multicloudj/registry/driver/OciRegistryClient.java 75.00% 1 Missing and 1 partial ⚠️

❌ Your patch status has failed because the patch coverage (78.09%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage.

Additional details and impacted files
@@             Coverage Diff              @@
##               main     #324      +/-   ##
============================================
+ Coverage     81.98%   81.99%   +0.01%     
- Complexity      520      552      +32     
============================================
  Files           181      183       +2     
  Lines         11119    11199      +80     
  Branches       1480     1494      +14     
============================================
+ Hits           9116     9183      +67     
- Misses         1340     1351      +11     
- Partials        663      665       +2     
Flag Coverage Δ
unittests 81.99% <78.09%> (+0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

@sharatchandrag sharatchandrag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plz check the test coverage, other than that lgtm

@iamabhilaksh iamabhilaksh force-pushed the registry/implement-aws-registry branch from 1a3b77d to 307d75e Compare March 5, 2026 10:19
*/
public OciRegistryClient(String registryEndpoint, AuthProvider authProvider) {
this(registryEndpoint, authProvider, null);
public OciRegistryClient(String registryEndpoint, AbstractRegistry registry) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iamabhilaksh I remember we had refactored this name from client to transport , was that PR closed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR was not closed as it had multiple overlapping code changes leading to serious merge conflicts. I'll be raising a separate PR for all the same :)

return host;
this.httpClient = builder.build();
}
this.tokenExchange = new BearerTokenExchange(this.httpClient);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wasn't the Bearer method provider specific ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That PR was not closed as it had multiple overlapping code changes leading to serious merge conflicts. I'll be raising a separate PR for all the same :)

Comment on lines +70 to +72
protected List<HttpRequestInterceptor> getInterceptors() {
return Collections.emptyList();
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice!

@iamabhilaksh
Copy link
Contributor Author

Note : Checkstyle config was updated and formatted code was pushed. This lead to merge conflicts and now the build is failing because of checkstyle errors.

registry.close(); // should not throw
}

static Stream<org.junit.jupiter.params.provider.Arguments>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there doesn't seems to be any conflicting definition

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in the follow-up revision.

@iamabhilaksh iamabhilaksh force-pushed the registry/implement-aws-registry branch from cf81cd1 to 14b4a3d Compare March 9, 2026 10:46
@sandeepvinayak sandeepvinayak merged commit 077997e into salesforce:main Mar 9, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants